Fix TSP None type encoding#4043
Conversation
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
|
@kinto0 has imported this pull request. If you are a Meta employee, you can view this in D110961669. |
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the core change (stop emitting the off-spec none builtin and encode None as a NoneType ClassType instead) is the right fix for #4035.
My main concern is that the NoneType location is re-derived by name and hardcoded to the types module, which diverges from pyrefly's authoritative, version-aware stdlib.none_type() on Python < 3.10. I'd suggest routing None through the real stdlib.none_type() class so the encoding is version-correct and identical to an explicit types.NoneType annotation. Details inline. It would also be worth an end-to-end Pylance check to confirm the hover symptom actually round-trips.
| PyreflyType::Any(_) => builtin("any"), | ||
| PyreflyType::Never(_) => builtin("never"), | ||
| PyreflyType::None => builtin("none"), | ||
| PyreflyType::None => self.types_class("NoneType", TypeFlags::INSTANCE), |
There was a problem hiding this comment.
The behavior change here is correct — builtin("none") was off-spec. My design concern is how the target is derived: types_class re-derives the NoneType location by name (types + "NoneType") instead of using pyrefly's authoritative stdlib.none_type(), which already resolves the version-correct location.
Could we instead convert the actual stdlib.none_type() ClassType through the existing convert_class_type(..., TypeFlags::INSTANCE) path? That makes None encode identically to an explicit types.NoneType annotation and inherits the correct module/range automatically, so there's no duplicated module/name/path logic. TypeConverter doesn't hold the stdlib today, but the server.rs caller does (it computes get_stdlib) and could pass the resolved class (or its qname) down. See find_definition_for_none in state/lsp.rs for the existing precedent that follows stdlib.none_type().
| let symbol = Name::new(name); | ||
| if let Some((module_path, lsp_range)) = self | ||
| .resolve_export | ||
| .and_then(|resolve| resolve(ModuleName::types(), &symbol)) |
There was a problem hiding this comment.
Concrete version-correctness issue with hardcoding ModuleName::types(): stdlib.none_type() resolves NoneType from types only on Python 3.10+, and from _typeshed on older versions (see none_location in crates/pyrefly_types/src/stdlib.rs). On a project targeting < 3.10 this lookup points at a different module than pyrefly's own None resolution / goto-definition, so the TSP declaration and the in-editor "go to definition" for None would disagree. Deriving the location from stdlib.none_type()'s qname avoids the divergence.
| /// Build a declaration for a class in `types.pyi`. | ||
| fn make_types_class_declaration(name: &str) -> RegularDeclaration { | ||
| let module_path = | ||
| pyrefly_python::module_path::ModulePath::bundled_typeshed(PathBuf::from("types.pyi")); |
There was a problem hiding this comment.
Minor, same caveat as the resolver path: this fallback hardcodes types.pyi, which is only correct for 3.10+. It only fires when no export resolver is available (unit tests), so impact is low — but if the location is derived from stdlib.none_type() this hardcoded file (and the whole helper) goes away.
| assert_eq!(name, Some("none"), "Expected builtin name 'none'"); | ||
| let declaration = result.get("declaration").expect("Expected declaration"); | ||
| let name = declaration.get("name").and_then(|v| v.as_str()); | ||
| assert_eq!(name, Some("NoneType"), "Expected class name 'NoneType'"); |
There was a problem hiding this comment.
This (and the unit tests) verify the wire shape — Class + name NoneType — but not the actual symptom from #4035: that Pylance now renders str | None instead of str | Unknown. The issue notes the consumer models this as builtins.NoneType; since we're emitting types.NoneType, it'd be reassuring to confirm end-to-end against a real Pylance client that the hover round-trips (i.e. that Pylance keys off the class name rather than the builtins module).
3 participants
Summary
Fixes #4035.
This changes Pyrefly's TSP conversion for
Nonefrom an off-specBuiltInTypenamednoneto aClassTypeinstance declared astypes.NoneType. The protocol'sBuiltInType.namecontract only allows sentinel names such asunknown,any,ellipsis,never, andnoreturn, so encodingNoneas a class keepsstr | Noneand other unions reconstructable by TSP consumers.The change also updates the TSP interaction coverage for
x = Noneso the actual wire response is checked as a class declaration rather than a builtin sentinel.Test Plan
cargo +stable-x86_64-pc-windows-gnu test -p pyrefly tsp::type_conversion::tests:: -- --nocapturecargo +stable-x86_64-pc-windows-gnu test -p pyrefly test_get_computed_type_none_is_class -- --nocapturecargo +stable-x86_64-pc-windows-gnu test -p pyrefly tsp::type_conversion::tests::test_function_declaration_resolves_special_function_via_export -- --exact --nocaptureRUSTUP_TOOLCHAIN=stable-x86_64-pc-windows-gnu python test.py --mode cargo --no-test --no-tensor-shapes --no-conformance --no-jsonschemagit diff --check